refactor(renderer): refactor renderer in multiple files#338
refactor(renderer): refactor renderer in multiple files#338
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the UI renderer into smaller modules (events/buffer/context) and updates RenderState’s indexing strategy, with corresponding test updates to use the new structure.
Changes:
- Split renderer responsibilities into
renderer.events,renderer.buffer, and a shared singletonrenderer.ctx. - Replace RenderState’s per-line index with sorted range arrays and add a cached max-line optimization.
- Add batch update support in
output_windowto reduce repeatedmodifiabletoggling during multi-step buffer updates.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/render_state_spec.lua | Updates assertions for new range-based validity flags and adds coverage for early-return paths. |
| tests/unit/permission_integration_spec.lua | Switches from renderer internals to renderer.events + renderer.ctx for integration-style behavior. |
| tests/unit/hooks_spec.lua | Routes hook tests through renderer.events handlers. |
| tests/unit/cursor_tracking_spec.lua | Updates scrolling tests to use ctx.prev_line_count and message update events. |
| tests/manual/renderer_replay.lua | Reads actions from renderer.ctx.render_state instead of renderer internals. |
| tests/helpers.lua | Captures actions via renderer.ctx.render_state. |
| lua/opencode/ui/renderer/events.lua | New module containing renderer event handlers (message/part/session/permission/question/etc.). |
| lua/opencode/ui/renderer/ctx.lua | New shared mutable renderer context (singleton via require cache). |
| lua/opencode/ui/renderer/buffer.lua | New module encapsulating buffer writes, diff-optimized part replacement, and rerender helpers. |
| lua/opencode/ui/renderer.lua | Becomes orchestrator: subscriptions, reset/teardown, full-session render, scrolling, and public accessors. |
| lua/opencode/ui/render_state.lua | Replaces line index maps with sorted ranges + binary search; adds snapshot-id index + max-line caching. |
| lua/opencode/ui/question_window.lua | Calls question rendering/clearing via renderer.events. |
| lua/opencode/ui/permission_window.lua | Calls permissions rendering via renderer.events. |
| lua/opencode/ui/output_window.lua | Adds begin_update/end_update batching and updates buffer validity checks. |
| lua/opencode/ui/debug_helper.lua | Uses renderer.ctx.render_state for message lookup. |
| lua/opencode/state.lua | Removes legacy wrapper module file (module now resolves via opencode/state/init.lua). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| function M.begin_update() | ||
| local windows = state.windows | ||
| if not windows or not windows.output_buf then | ||
| return false | ||
| end | ||
| if _update_depth == 0 then | ||
| _update_buf = windows.output_buf | ||
| vim.api.nvim_set_option_value('modifiable', true, { buf = _update_buf }) | ||
| end | ||
| _update_depth = _update_depth + 1 | ||
| return true |
There was a problem hiding this comment.
begin_update() toggles modifiable without verifying the output buffer is still valid. If state.windows.output_buf is stale/deleted, nvim_set_option_value will error and can break rendering. Consider checking vim.api.nvim_buf_is_valid(windows.output_buf) (or M.buffer_valid) before setting options, and wrapping the option set in pcall similarly in end_update() to avoid leaving _update_depth/modifiable in a bad state.
| cached.has_extmarks = next_has_extmarks | ||
|
|
||
| if next_has_actions then | ||
| ctx.render_state:add_actions(part_id, formatted_data.actions, cached.line_start + 1) |
There was a problem hiding this comment.
add_actions() offset here uses cached.line_start + 1, but write_formatted_data() registers actions with an offset equal to the 0-based buffer start line (target_line). Since formatter action ranges are already 0-based (see formatter helper comments) and get_actions_for_line is called with 0-based line numbers, this + 1 will shift actions down by one line on re-render. Use the same offset as initial writes (i.e., cached.line_start) to keep action hit-testing consistent.
| ctx.render_state:add_actions(part_id, formatted_data.actions, cached.line_start + 1) | |
| ctx.render_state:add_actions(part_id, formatted_data.actions, cached.line_start) |
| output_window.clear_extmarks(cached.line_start - 1, cached.line_end) | ||
| output_window.set_lines({}, cached.line_start - 1, cached.line_end) |
There was a problem hiding this comment.
output_window.set_lines uses Neovim's 0-based, end-exclusive indexing. In remove_part, passing end_line = cached.line_end will not remove the final line of the part (it removes up to line_end-1). This can leave a trailing line behind and desync render_state from the actual buffer. Use an end index of cached.line_end + 1 (and update the extmark clear range accordingly).
| output_window.clear_extmarks(cached.line_start - 1, cached.line_end) | |
| output_window.set_lines({}, cached.line_start - 1, cached.line_end) | |
| output_window.clear_extmarks(cached.line_start, cached.line_end + 1) | |
| output_window.set_lines({}, cached.line_start, cached.line_end + 1) |
| output_window.clear_extmarks(cached.line_start - 1, cached.line_end) | ||
| output_window.set_lines({}, cached.line_start - 1, cached.line_end) |
There was a problem hiding this comment.
Same end-exclusive indexing issue as remove_part: clear_extmarks/set_lines are called with end_line = cached.line_end, which will leave the last header line in the buffer. This can corrupt subsequent inserts/shifts and leave orphaned lines/extmarks. Use cached.line_end + 1 for end-exclusive API calls (and keep the line_start - 1 behavior only if you intentionally want to remove the separator line).
| output_window.clear_extmarks(cached.line_start - 1, cached.line_end) | |
| output_window.set_lines({}, cached.line_start - 1, cached.line_end) | |
| output_window.clear_extmarks(cached.line_start - 1, cached.line_end + 1) | |
| output_window.set_lines({}, cached.line_start - 1, cached.line_end + 1) |
| self:shift_all(shift_from, -line_count) | ||
|
|
||
| return true | ||
| end | ||
|
|
||
| ---Shift all content starting from a line by delta | ||
| ---Optimized to scan in reverse order and exit early | ||
| ---@param from_line integer Line number to start shifting from | ||
| ---@param delta integer Number of lines to shift (positive or negative) | ||
| local function shift_action(action, delta) | ||
| if action.display_line then | ||
| action.display_line = action.display_line + delta | ||
| end |
There was a problem hiding this comment.
The docstring above shift_all still claims it is "optimized to scan in reverse order", but the implementation now iterates over pairs(self._messages) / pairs(self._parts) and no longer scans in reverse. Please update the comment to match the current behavior (it still has an early-return optimization via _get_max_line_end()).
No description provided.